An impressive re-engineering work ...caused by the annoying fact that the big players are all creating their own vendor lock-in solutions.

Regarding cue shift I've tested the following file in Mixxx:

digital-dj-tools/dj-data-converter#3 (comment)

Both libmad (default) and the new FFmpeg 4.x decoder (#1356) produce the same output for this file, i.e. no cue shift in Mixxx:

SoundSourceMP3 (libmad) - cue

SoundSourceFFmpeg4 - cue

I have to check why SoundSourceMP3 and SoundSourceFFmpeg4 slightly disagree about the actual length of the audio stream by about 50 ms:

SoundSourceMP3 (libmad) - end

SoundSourceFFmpeg4 - end

In SoundSourceFFmpeg4 we are using the (estimated?) value that FFmpeg reports for the stream while in SoundSourceMP3 we calculate the length on our own by visiting all valid MP3 frame headers and summing up their length (using mad_timer_count).

If you have another file that is supposed to cause a cue shift in Mixxx I would be interested in testing and analyzing it. We are trying hard to achieve sample accurate decoding.

Thank you for contributing this rather complex integration. Even though I don't use recordbox myself, I expect many users to welcome this new feature. Interoperability should indeed become a key feature of Mixxx!

I just added some technical comments and did not review the actual code yet.

Happy to contribute, kudos to all devs involved for a great piece of software. I also learnt a good deal attempting this integration.

Have addressed all technical comments.

In regards to the "cues shifted in time" issue, what I encountered may indeed be related to this 50ms discrepancy between SoundSourceMP3 and SoundSourceFFmpeg4. I initially compensated for the off timings by hard-coding a shift of about 50ms. After trying to find more information about this, I found the #1411 and digital-dj-tools/dj-data-converter#3 threads, and assumed this was the same issue. I thought I had not incorporated the frame size (eg. kFrameSize in beatmap.cpp) and assumed this was the same 26ms out (52ms with kFrameSize).

Here are some comparisons on a shifted track with a hotcue manually set in Rekordbox on the first beat, have sent it to you via Zullip to analyze:

Audacity shows the starting beat at 1.053ms, the hotcue point read by this new Rekordbox library feature from the PDB database file is 1.054ms, however in Mixxx the first beat is at 1.00ms, the track appears shifted 50ms to the left. The read hotcue is correctly located in Mixxx at 1.05ms.

I am developing on macOS 10.13.4 and using macOS-provided MP3 codec.

@ehendrikd Please upload the example file somewhere and send me an e-mail with the link.

We also need your permission by signing our Contributor Agreement.

Great opportunity to improve our interoperability story on many different levels! I started developing a new standalone library backend running as a (local or remote) web service that will hopefully cover all use cases. It might not only be used for managing our library, but also for synchronizing external repositories and databases. Disclaimer: I really need to rewrite the README, please refer to the Zulip topic for some details and examples.

@ehendrikd Got the link ;)

Draws heavily from the hard work completed here:
Uses the C++ Kaitai Struct binary parsing libraries:

We have to be extremely careful about including new libraries and adapted code in Mixxx, because of our somewhat unique license (app store exception). We will have to get permission from the license holders for both the crate-digger project and this kaitai struct library in order to include this PR in Mixxx. The best case would be if they can sign the contributor agreement, but short of that we'll need to determine if their licenses are compatible. What are the licenses for those two projects?

Hello! I’m the author of the Crate Digger project that was used to help implement this, and the author called this PR to my attention. I replied in more depth to his email inquiry, but figured I could weigh in here as well. As far as I can tell, there is no license issue: Although Crate Digger itself is released under the Eclipse Public License 1.0, which—even though it is a permissive open-source license—has some subtle and complex incompatibilities with GPLv2, I don’t think my license matters. This submission does not contain any of my source code. It contains some C++ classes that were generated with the help of my source code and the Kaitai Struct compiler, and another class which was inspired by one of mine, but they are independent works.

The generated classes do rely on a Kaitai Struct C++/STL runtime library, but that is licensed by the Kaitai Struct project under the MIT License, which is compatible with release under GPLv2. (I will let them speak more authoritatively about this, however.)

If it would nonetheless ease your mind to have me sign a contributor agreement, I can certainly read that and make sure it would not harm my ability to continue taking my own projects in the directions I want to go with them.

Actually, I just saw the link to the contributor agreement above, and it is short and very reasonable, so I went ahead and signed it to make sure there are no issues with respect to this PR’s use of Crate Digger to generate source code. So as long as you are fine with the Kaitai Struct C++/STL runtime’s MIT License, you are good to go. If you want a signature from them as well, that’s up to them. 😄 In my experience, though, they are friendly and helpful (and they release their compiler under GPLv3+ themselves), so hopefully that will be forthcoming if needed.

By the way, this looks like a really cool project! If you ever want to add Master/Sync compatibility with Pioneer players, I have figured out how to do that as well. My implementation would not be of direct use to you, since it is a Java library, but I have published all the details of the protocol in a platform-independent analysis document, as part of my dysentery project: https://github.com/Deep-Symmetry/dysentery/blob/master/doc/Analysis.pdf

Thank you @brunchboy for commenting and signing the contributor agreement, Members of the Kaitai Struct projects have welcomed the use of the Kaitai Struct C++/STL runtime in this project, and suggested that as they haven't contributed directly to the project that they don't need to sign the contributor agreement, as long as the Kaitai Struct C++/STL runtime licence remains (which I included in the PR here: https://github.com/ehendrikd/mixxx/blob/rekordbox-device-library-feature/lib/kaitai/LICENSE). So as long as the MIT license included is OK, there does not seem to be any other licencing issues.

I have investigated the "cue offset issue" further and have some results, however still not reliable enough to include as yet, and may not be for some time. To reiterate, the code for all the above discussions about this issue are not included in this PR, and thus should be ready to pull limited to reading just tracks, folders and playlists.

Hi @ehendrikd and @uklotzde
thanks for your interest on the mp3 cues shift investigation.

I've now made a "give me the numbers" summary. Please find it in this exact comment
Hopefully it explains clearly what we investigated over several months.

I encountered the same "cues shifted in time" issue discussed in #1411 and digital-dj-tools/dj-data-converter#3.
Interestingly when analyzing these "bad" MP3s with (Pedro script) they were all classed as "D" - no correction needed.

Comments:

• All investigations we done were on Traktor->Rekordbox.
  • The MP3 format is so poor that the RB->MIXXX can easily be different
• Case D has 16% false positive rate. This is the biggest for all the groups
  • In particular we've seen offline that encoding the very latest FFMPEG would produce a FP

So clearly more investigation is needed

Thank you for the update @pestrela.

Just wondering if this is now ready to merge? Have not heard any further feedback.

Thank you for the update @pestrela.

Interestingly when analyzing these "bad" MP3s with (Pedro script) they were all classed as "D" - no correction needed.

small update: we now removed these False Positives, a lot, by using different tools for different tasks (revised algo below)
This is important because fresh FFMPEG encoded files were being incorrectly classified.

In a sentence, eyeD3 was not updated, while mp3guessenc was - and this matches the TK vs RB mp3 situation

Current algorithm

if ! (mp3guessenc sees Xing/INFO tag):
    case = "A"
    correction = 0ms

else if ! eyeD3 sees correct LAME tag:
    case = "B"
    correction = 26ms
         
else if eyeD3 sees correct LAME tag:
    case = "D"
    correction = 0ms

I agree, opening a dialog window to pick the hotcue color would be annoying. So lets keep the ColorMenu as it is in #2238 and just add one entry to open the QColorDialog if the user wants anything other than the 8 default colors we have already.

I totally agree, sorry I forgot that PR already contains such ColorMenu.

But, where to configure the palette? If it's not in preferences, nor in ColorMenu, then where?

So we should split the PR into separate parts. One where we can lay the groundwork so this PR can be merged and another one so we can improve the UX.

QColorDialog already provides an "Add to custom colors" button. As far as I understand from the QColorDialog documentation, all Mixxx has to do is save/restore the custom colors on shutdown/startup using QColorDialog's static methods.

@daschuer would the solution I summarized in my comment above work for you given that ...

Yes, that works. I try to summarize the features / requirements:

• Allow to store any color in the database
• Provide a good accessible named color palette by default
• Allow to use custom colors

The cue point migration path is quite unclear for me. We may import the original colors to allow a Mixxx round trip without altering the original colors. That leads to up to 64 additional colors in the database. They will either clutter the color picker, or are hard to access for new CUEs.

Alternative we may allow to map the external colors to one of the predefined colors or a custom color.

We could provide an option to replace colors from other software with the closest color in the default Mixxx 8 color palette. However, I don't think this matters much if we follow @Holzhaus's and @brunchboy's idea for controller scripts to match arbitrary colors from hotcues to the closest color in the controller's limited color palette.

Here is how I think this could work:

1. In the mapping script, define an object that maps RGB colors to MIDI codes, for example if the controller could be sent a MIDI value of 1 to set an LED red, 2 to set it to blue, 3 to set it to green:

exampleController.colors = {
    1: [255, 0, 0],
    2: [0, 255, 0],
    3: [0, 0, 255],
}

2. Provide a JS function, let's say color.registerControllerColors, that takes the exampleController.colors object.
3. When a controller script sets a colored LED, it gets the RGB color value for the hotcue from a ControlObject. The script passes this to a function, let's say color.midiForClosestColor, which returns the MIDI code for the closest color provided in step 2.

The solutions seems good. The only Issue I see as a mapper is that I don't know how I would accurately reverse engineer the proprietary palette of my controller in case it is not documented (as it is the case with pretty much all numark hardware).

@Swiftb0y I isn't documented for most controllers IMHO. I could upload a list of colors supported by Serato, e.g.:

If your controller supports all of them, just only need to find the mapping between MIDI values and these RGB codes.

As pretty much every controller is "serato certified" nowadays, I am pretty sure it supports all of those colors. Most controllers however actually have at least 64 colors in their default factory palette which we could take advantage off. It would be nice if there was an accurate way to map these to some colors values which are somewhat equal on an sRGB screen.

I added some documentation about the colors that serato supports to my repo: https://github.com/Holzhaus/serato-tags/blob/master/docs/serato_markers2.md#colors

QColorDialog already provides an "Add to custom colors" button. As far as I understand from the QColorDialog documentation, all Mixxx has to do is save/restore the custom colors on shutdown/startup using QColorDialog's static methods.

I see. That can work. The only weird thing I see to this solution is that in order to change the palette without changing any hotcue color, the user must:

1. Load a track
2. Set a hotcue if the track has none
3. Open the ColorMenu for that cue
4. Select the "Other color" button to open the QColorDialog
5. Set or change a color on the palette
6. Remember to press cancel, otherwise the hotcue color will be modified.

If we had the palette in preferences, the process would be:

1. Open preferences
2. Go to the color palette page
3. Set or change a color on the palette
4. Apply changes or press accept

The later seems more intuitive to me

The cue point migration path is quite unclear for me

When importing hotcues, we don't add the colors to the palette by default, that's the whole point of this approach. We can open a dialog or add a right-click menu option to add the colors of some tracks to the palette.

Alternatively, we can offer several pre-made palettes and let the user select them. One for each major dj software like Serato and Rekordbox.

@ehendrikd

Chose to map the 17 (including default) hotcue colors Rekordbox uses to the closest 11 Mixxx predefined colors,

If this is done, then this PR should be ready to merge. When we actually implement the new color solution, the current PredefinedColors will be gone and we will need to refactor the code in this PR using them.

There's no reason to block this PR now.

@ferranpujolcamins do you have any code committed yet? If so, let's continue this discussion in a new PR. Otherwise we can continue on Zulip. This has gotten way off topic from the subject of this PR.

Chose to map the 17 (including default) hotcue colors Rekordbox uses to the closest 11 Mixxx predefined colors,

It may be good to offer this as an option, but by default I don't think importing cues should change their color.

do you have any code committed yet?

Not yet, I'll start working ASAP

Have temporarily commented out setting the hotcue colors so this PR can be merged. Will open a new PR to implement the hotcue colors once the proposed custom colors/palette PR is completed and merged. Thank you to everyone contributing to proposing and implementing a solution.

Also note this PR depends on #2103 being merged, due to a bug found involving cue sources. #2103 should fix the bug found as it removes the cue sources.

Good idea. I think this is ready to merge as soon as #2103 is merged then unless anyone else wanted to review the code here.

A merge conflict has developed.

@ehendrikd #2103 has been merged, so we can merge this as soon as you fix the merge conflict.

I get a build error:

In file included from src/library/library.cpp:32:
src/library/rekordbox/rekordboxfeature.h:68:44: error: only virtual member functions can be marked 'override'
            KeyboardEventFilter* keyboard) override;

Thank you @ehendrikd!

In the future, please check that commits compile before pushing them (unless there is a build error you need help with). This especially applies to merge commits. Just because all merge conflicts are resolved does not mean that the code compiles.